New optional column: item_subtype #1143

jaredbeck · 2018-08-22T04:34:00Z

Gives PT-AT the data it needs to fix #594. PT-AT will make changes like:

# paper_trail_association_tracking/reifiers/has_one.rb
# def load_versions(assoc, model, transaction_id, version_at, base_class_name)
            where("#{version_table_name}.item_type = ?", base_class_name).
+           where("#{version_table_name}.item_subtype = ?", assoc.class_name).
            where("created_at >= ? OR transaction_id = ?", version_at, transaction_id).

I have tested the above with the "cars and bicycles" example in person_spec.rb and it passes.

Replaces (reverts) #1108, which has proved problematic in #1135, leading to further proposed increases in complexity such as seanlinsley#1. @lorint I know you worked hard on #1108, and I did too, but if this is actually the simpler approach, as I propose, I hope we can both approach this objectively. I've never had to revert something as large as #1108 and I don't suggest it lightly.

I haven't written the docs or the migration to populate item_subtype. I would like to get your opinions on this approach first.

I made this an optional column because it is my understanding that this issue (#594) only affects PT-AT users with STI, a very small subset of our users.

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - #1135 - #1137 - seanlinsley#1

If present, will be populated with subclass name. This will be used by PT-AT.

lorint

Would be nice to have a generator similar to update_sti that fills in item_subtype for historic records.

lorint · 2018-08-22T04:51:13Z

spec/generators/paper_trail/install_generator_spec.rb

-            }
-          }
-        }
-      )


Might as well at least keep this enhancement to the spec!

lorint · 2018-08-22T04:53:50Z

spec/models/person_spec.rb

@@ -5,10 +5,8 @@
 # The `Person` model:
 #
 # - has a dozen associations of various types
-# - has a custom serializer, TimeZoneSerializer, for its `time_zone` attribute
+# - has a custome serializer, TimeZoneSerializer, for its `time_zone` attribute


lorint · 2018-08-22T12:46:08Z

If you're committed to going in this direction then it might be somewhat moot at this point, but I've pushed an update to Sean's PR#1 based on your review.

jaredbeck · 2018-08-22T17:36:04Z

Lorin, it sounds like you agree that this will give PT-AT the data it needs to solve Reify on associations fails if using Single Table Inheritance #594. Is that correct?
Sean, can you test your app against this branch? I assume that, because I've reverted Fix for issue #594, reifying sub-classed models that use STI #1108, that this branch fixes your issue Unable to join versions association #1135.

seanlinsley · 2018-08-22T18:28:24Z

I'll test this as soon as I can, but in the meantime these tests should verify the behavior if you'd like to add them to this PR:

seanlinsley · 2018-08-23T00:09:26Z

This PR fixes both issues 🙌

#1143 (comment)

jaredbeck · 2018-08-23T02:32:40Z

.. in the meantime these tests should verify the behavior if you'd like to add them ..

Tests added.

seanlinsley

This is probably the most maintainable fix for #594 since it doesn't require patching Rails. @lorint what do you think about the approach in general? (save from it not having a migration generator)

BTW the test failures can be fixed by adding group(:id)

seanlinsley · 2018-08-23T19:33:01Z

We're now using this branch in production & will likely remove our object column next week.

Thank you both for all the time you've put into the upcoming release 💛

jaredbeck · 2018-08-24T16:05:10Z

@lorint does this work for you? Does it provide PT-AT with the data it needs to solve #594? Please let me know if you have any objections. If I don't hear from you, I'll probably merge this weekend.

lorint · 2018-08-26T15:25:37Z

It does have the data we would need, and potentially covers Rick Tyler's interest around the store_base_sti_class gem.

It does require a fair bit of rework internally, doing checks around item_type and item_subtype. And around 16GB of additional storage requirement for this new column. But this will get offset by the possible removal of the object column in the near future.

We are considering both removing object and implementing the item_type code from PR#1137, in which case we could backport our fixes that use item_type for reification to PT-AT using item_subtype.

jaredbeck · 2018-08-27T01:04:40Z

It does have the data we would need, and potentially covers Rick Tyler's interest around the store_base_sti_class gem.

Great, thanks.

And around 16GB of additional storage requirement for this new column.

If y'all are concerned about disk space, we could choose to store item_subtype only when it differs from item_type.

jaredbeck · 2018-08-27T01:06:55Z

We're now using this branch in production.

I usually delete topic branches immediately after merging, so please switch to master ASAP.

[ci skip]

lorint · 2018-08-27T03:08:57Z

If y'all are concerned about disk space, we could choose to store item_subtype only when it differs from item_type.

For us the disk space is of interest only in light of how far-reaching it is to have a half billion records loaded into RAM at once. We've got 5 covering indexes that leverage item_type, and there's some right magic we're able to unfold for our end users based on this, 2 of these 5 b-tree indexes have item_type as their primary index. Hence any even slight update to behaviour around item_type can mean significant impact on a fair range of tests, and around the amount of work the database engine goes through overall. No worries about extra overhead during CUD operations. Entire focus is on finding related transactions quickly. For us in these indexes we will continue to store real class names as opposed to base_class so we can have just one column involved, and maintain consistent sub-second searches on item_type.

jaredbeck · 2018-08-27T03:23:17Z

For us the disk space is of interest only in light of how far-reaching it is to have a half billion records loaded into RAM at once.

Dang. And I thought Sean has a lot of records 😅

.. 2 of these 5 b-tree indexes have item_type as their primary index ..

db indexing practices are a personal interest of mine. I'd love to hear more about the shape of the queries and brainstorm about your index design. If it's not private IP, feel free to email me and we can chat more about about it.

paper-trail-gem#1143 (comment)

[ci skip]

v10.0.0 * tag 'v10.0.0': (40 commits) Release 10.0.0 Testing the :skip option allow `object_changes_adapter` to use the default behavior Lint: Improve Metrics/AbcSize from 22 to 21 Docs: which class attributes are public/private Docs: installation Docs: Update changelog and readme re: paper-trail-gem#1143 Generator to update historic item_subtype entries (paper-trail-gem#1144) Testing joins, as recommended by Sean Add optional column: item_subtype Revert paper-trail-gem#1108 (lorint's STI fix) Lint: RSpec/EmptyLineAfterExampleGroup Update development dependencies Lint: RSpec/InstanceVariable in model_spec, ctn'd Code style re: errors Docs: Fix link to bug report Add association tracking removal exception Readme fix: caller.find {} rather than caller.first {} Do not require PT-AT Docs: Organizing the changelog for 10.0.0 ...

Revert #1108 (lorint's STI fix)

9f004a6

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - #1135 - #1137 - seanlinsley#1

jaredbeck mentioned this pull request Aug 22, 2018

Make STI .joins() fix more consistent with existing tests seanlinsley/paper_trail#1

Open

Add optional column: item_subtype

e907295

If present, will be populated with subclass name. This will be used by PT-AT.

jaredbeck force-pushed the item_subtype branch from ac6443c to e907295 Compare August 22, 2018 04:43

jaredbeck requested a review from seanlinsley August 22, 2018 04:45

lorint reviewed Aug 22, 2018

View reviewed changes

jaredbeck added a commit that referenced this pull request Aug 23, 2018

Testing joins, as recommended by Sean

4b253c2

#1143 (comment)

jaredbeck force-pushed the item_subtype branch from 4b253c2 to 934c1d9 Compare August 23, 2018 02:01

jaredbeck added a commit that referenced this pull request Aug 23, 2018

Testing joins, as recommended by Sean

934c1d9

#1143 (comment)

Testing joins, as recommended by Sean

8b4ec38

#1143 (comment)

jaredbeck force-pushed the item_subtype branch from 934c1d9 to 8b4ec38 Compare August 23, 2018 02:18

seanlinsley approved these changes Aug 23, 2018

View reviewed changes

TylerRick mentioned this pull request Aug 23, 2018

Causes PaperTrail "destroy" versions to not be found appfolio/store_base_sti_class#37

Open

jaredbeck merged commit 0db05a2 into master Aug 27, 2018

jaredbeck deleted the item_subtype branch August 27, 2018 01:08

This was referenced Aug 27, 2018

Unable to join versions association #1135

Closed

Fix joins(:versions) #1137

Closed

Fix for PT issue #594 westonganger/paper_trail-association_tracking#5

Closed

jaredbeck added a commit that referenced this pull request Aug 27, 2018

Docs: Update changelog and readme re: #1143

7b44228

[ci skip]

aried3r pushed a commit to aried3r/paper_trail that referenced this pull request Dec 14, 2020

Testing joins, as recommended by Sean

450f1c0

paper-trail-gem#1143 (comment)

aried3r pushed a commit to aried3r/paper_trail that referenced this pull request Dec 14, 2020

Docs: Update changelog and readme re: paper-trail-gem#1143

6928f91

[ci skip]

aried3r mentioned this pull request Jul 21, 2021

Improve fetching reified class names for STI models #1332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New optional column: item_subtype #1143

New optional column: item_subtype #1143

jaredbeck commented Aug 22, 2018 •

edited

Loading

lorint left a comment

lorint Aug 22, 2018

lorint Aug 22, 2018

lorint commented Aug 22, 2018

jaredbeck commented Aug 22, 2018

seanlinsley commented Aug 22, 2018

seanlinsley commented Aug 23, 2018

jaredbeck commented Aug 23, 2018

seanlinsley left a comment

seanlinsley commented Aug 23, 2018

jaredbeck commented Aug 24, 2018

lorint commented Aug 26, 2018

jaredbeck commented Aug 27, 2018

jaredbeck commented Aug 27, 2018

lorint commented Aug 27, 2018

jaredbeck commented Aug 27, 2018

New optional column: item_subtype #1143

New optional column: item_subtype #1143

Conversation

jaredbeck commented Aug 22, 2018 • edited Loading

lorint left a comment

Choose a reason for hiding this comment

lorint Aug 22, 2018

Choose a reason for hiding this comment

lorint Aug 22, 2018

Choose a reason for hiding this comment

lorint commented Aug 22, 2018

jaredbeck commented Aug 22, 2018

seanlinsley commented Aug 22, 2018

seanlinsley commented Aug 23, 2018

jaredbeck commented Aug 23, 2018

seanlinsley left a comment

Choose a reason for hiding this comment

seanlinsley commented Aug 23, 2018

jaredbeck commented Aug 24, 2018

lorint commented Aug 26, 2018

jaredbeck commented Aug 27, 2018

jaredbeck commented Aug 27, 2018

lorint commented Aug 27, 2018

jaredbeck commented Aug 27, 2018

jaredbeck commented Aug 22, 2018 •

edited

Loading